Skip to content

[STACKED] Reject relaxed bounds inside associated type bounds (ATB) #135331

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

fmease
Copy link
Member

@fmease fmease commented Jan 10, 2025

Caution

STACKED on top of PR #142693 (approved, in the queue). Only the last commit is relevant!

Reject relaxed bounds — most notably ?Sized — inside associated type bounds TraitRef<AssocTy: …>.

This was previously accepted without warning despite being incorrect: ATBs are not a place where we perform sized elaboration, meaning TraitRef<AssocTy: …> does not elaborate to TraitRef<AssocTy: Sized + …> if doesn't contain ?Sized. Therefore ?Sized is meaningless. In no other (stable) place do we (intentionally) allow relaxed bounds where we don't also perform sized elab, this is highly inconsistent and confusing! Another point of comparison: For the desugared $SelfTy: TraitRef, $SelfTy::AssocTy: … we don't do sized elab either (and thus also don't allow relaxed bounds).

Moreover — as I've alluded to back in #135841 (review) — some later validation steps only happen during sized elaboration during HIR ty lowering1. Namely, rejecting duplicates (e.g., ?Trait + ?Trait) and ensuring that Trait in ?Trait is equal to Sized2. As you can probably guess, on stable/master we don't run these checks for ATBs (so we allow even more nonsensical bounds like Iterator<Item: ?Copy> despite T-types's ruling established in the FCP'ed #135841).

This PR rectifies all of this. I cratered this back in 2025-01-10 with (allegedly) no regressions found (report, its analysis). However a contributor manually found two occurrences of TraitRef<AssocTy: ?Sized> in small hobby projects (presumably via GH code search). I immediately sent downstream PRs: Gui-Yom/turbo-metrics#14, ireina7/summon#1 (however, the owners have showed no reaction so far).

I'm leaning towards banning these forms without a FCW because a FCW isn't worth the maintenance cost3. Note that associated type bounds were stabilized in 1.79.0 (released 2024-06-13 which is 13 months ago), so the proliferation of ATBs shouldn't be that high yet. If you think we should do another crater run since the last one was 6 months ago, I'm fine with that.

Fixes #135229.

Footnotes

  1. I consider this a flaw in the implementation and I've already added a huge FIXME.

  2. To be more precise, if the internal flag -Zexperimental-default-bounds is provided other "default traits" (needs internal feature lang_items) are permitted as well (cc closely related internal feature: more_maybe_bounds).

  3. Having to track this and adding an entire lint whose remnants would remain in the code base forever (we never fully remove lints).

@fmease fmease added the rla-silenced Silences rust-log-analyzer postings to the PR it's added on. label Jan 10, 2025
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 10, 2025
@fmease
Copy link
Member Author

fmease commented Jan 10, 2025

@bors try

@fmease fmease added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 10, 2025
@bors
Copy link
Collaborator

bors commented Jan 10, 2025

⌛ Trying commit 5c245de with merge de39175...

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 10, 2025
@bors
Copy link
Collaborator

bors commented Jan 10, 2025

☀️ Try build successful - checks-actions
Build commit: de39175 (de39175d9e0f63ff8967b81f9639ba28d11ae6ef)

@fmease
Copy link
Member Author

fmease commented Jan 10, 2025

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-135331 created and queued.
🤖 Automatically detected try build de39175
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. labels Jan 10, 2025
@craterbot
Copy link
Collaborator

🚧 Experiment pr-135331 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-135331 is completed!
📊 117 regressed and 36 fixed (566362 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jan 11, 2025
@fmease fmease added S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2025
@Skgland
Copy link
Contributor

Skgland commented Jan 11, 2025

Based on running Crater Analysis on this crater report
110 out of 117 build failed (unknown) regressions appear spurious caused by running out of disk space, i.e. their logs contain the string no space left on device.
The remaining seven appear to be spurious as well, based on manual review of the logs.

full automated report
Regressed: 117
build failed(unknown): 117
----------------------------------
Results:
no-space: 110
----------------------------------
sum: 110
others: 7
----------------------------------
[
    (
        "ceronman.writing-a-c-compiler.ce987db61026febea0e12806e99932b735808d61",
        "try%23de39175d9e0f63ff8967b81f9639ba28d11ae6ef/gh/ceronman.writing-a-c-compiler",
    ),
    (
        "benrussell.simple_open-rs.feb28e8321d6c3db2cd5b20a6c009c70db767118",
        "try%23de39175d9e0f63ff8967b81f9639ba28d11ae6ef/gh/benrussell.simple_open-rs",
    ),
    (
        "dennisoelkers.brghtnss.43a8295abe3b93dfc257f7908710ef7bb4d2675e",
        "try%23de39175d9e0f63ff8967b81f9639ba28d11ae6ef/gh/dennisoelkers.brghtnss",
    ),
    (
        "gauteh.hidefix.5569aa88689b7dd1867354103e0485894b0d06ec",
        "try%23de39175d9e0f63ff8967b81f9639ba28d11ae6ef/gh/gauteh.hidefix",
    ),
    (
        "smasher164.judgefmt.945d1d88d4083018bcd997df019a03656bc24865",
        "try%23de39175d9e0f63ff8967b81f9639ba28d11ae6ef/gh/smasher164.judgefmt",
    ),
    (
        "pillow-routing-0.4.2",
        "try%23de39175d9e0f63ff8967b81f9639ba28d11ae6ef/reg/pillow-routing-0.4.2",
    ),
    (
        "tree-sitter-po-0.0.1",
        "try%23de39175d9e0f63ff8967b81f9639ba28d11ae6ef/reg/tree-sitter-po-0.0.1",
    ),
]

rust-bors bot added a commit that referenced this pull request Jun 20, 2025
More robustly deal with relaxed bounds and improve their diagnostics

Scaffolding for #135229 (CC #135331)

Fixes #136944.
Fixes #142718.
rust-bors bot added a commit that referenced this pull request Jun 28, 2025
More robustly deal with relaxed bounds and improve their diagnostics

Scaffolding for #135229 (CC #135331)

Fixes #136944.
Fixes #142718.
fmease added 7 commits July 18, 2025 03:13
Only relevant to the internal feature `more_maybe_bounds`.
We no longer move trait predicates where the self ty is a ty param to
"the bounds of a ty param".
* The phrasing "only does something for" made sense back when this
  diagnostic was a (hard) warning. Now however, it's simply a hard
  error and thus completely rules out "doing something".
* The primary message was way too long
* The new wording more closely mirrors the wording we use for applying
  other bound modifiers (like `const` and `async`) to incompatible
  traits.
* "all other traits are not bound by default" is no longer accurate
  under Sized Hierarchy. E.g., traits and assoc tys are (currently)
  bounded by `MetaSized` by default but can't be relaxed using
  `?MetaSized` (instead, you relax it by adding `PointeeSized`).
* I've decided against adding any diagnositic notes or suggestions
  for now like "trait `Trait` can't be relaxed as it's not bound by
  default" which would be incorrect for `MetaSized` and assoc tys
  as mentioned above) or "consider changing `?MetaSized` to
  `PointeeSized`" as the Sized Hierarchy impl is still WIP)
Having multiple relaxed bounds like `?Sized + ?Iterator` is actually *fine*.
We actually want to reject *duplicate* relaxed bounds like `?Sized + ?Sized`
because these most certainly represent a user error.

Note that this doesn't mean that we accept more code because a bound like
`?Iterator` is still invalid as it's not relaxing a *default* trait and
the only way to define / use more default bounds is under the experimental
and internal feature `more_maybe_bounds` plus `lang_items` plus unstable
flag `-Zexperimental-default-bounds` (historical context: for the longest
time, bounds like `?Iterator` were actually allowed and lead to a hard
warning).

Ultimately, this simply *reframes* the diagnostic. The scope of
`more_maybe_bounds` / `-Zexperimental-default-bounds` remains unchanged
as well.
@fmease fmease added the needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. label Jul 18, 2025
@fmease fmease changed the title [crater-only] Ban assoc ty unbounds Reject relaxed bounds inside associated type bounds (ATB, TraitRef<AssocTy: …>) Jul 18, 2025
@fmease fmease changed the title Reject relaxed bounds inside associated type bounds (ATB, TraitRef<AssocTy: …>) [STACKED] Reject relaxed bounds inside associated type bounds (ATB, TraitRef<AssocTy: …>) Jul 18, 2025
@fmease fmease changed the title [STACKED] Reject relaxed bounds inside associated type bounds (ATB, TraitRef<AssocTy: …>) [STACKED] Reject relaxed bounds inside associated type bounds (ATB) Jul 18, 2025
@fmease fmease force-pushed the ban-assoc-ty-unbounds branch from 5c245de to e7050cb Compare July 18, 2025 10:58
@fmease fmease added T-types Relevant to the types team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-experimental Status: Ongoing experiment that does not require reviewing and won't be merged in its current state. rla-silenced Silences rust-log-analyzer postings to the PR it's added on. labels Jul 18, 2025
@fmease
Copy link
Member Author

fmease commented Jul 18, 2025

r? types

@fmease fmease marked this pull request as ready for review July 18, 2025 11:42
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2025
@lcnr
Copy link
Contributor

lcnr commented Jul 18, 2025

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jul 18, 2025

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 18, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 18, 2025
…er-errors

More robustly deal with relaxed bounds and improve their diagnostics

Scaffolding for rust-lang#135229 (CC rust-lang#135331)

Fixes rust-lang#136944 (6th commit).
Fixes rust-lang#142718 (8th commit).
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 18, 2025
…er-errors

More robustly deal with relaxed bounds and improve their diagnostics

Scaffolding for rust-lang#135229 (CC rust-lang#135331)

Fixes rust-lang#136944 (6th commit).
Fixes rust-lang#142718 (8th commit).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-types Relevant to the types team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Relaxed bounds in associated type bounds don't get rejected
7 participants